Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversion functions for authStore types #96

Merged
merged 8 commits into from
Aug 3, 2023

Conversation

tomasciccola
Copy link
Contributor

@tomasciccola tomasciccola commented Aug 2, 2023

Things to consider:

  • what fields are required?
    • Role: every field
    • Device: every field
    • coreOwnership: every field
  • Device: actions are strings. When we know possible actions, it should be an enum.
  • action name delimiters:
    protobuf doesn't allow colons (':') on identifiers, so to avoid type problems I basically replaced that delimiter with underscore ('_'). So in schemas role:set becomes role_set. Another separator could be chosen, or a convertion function (convertAction) could be written; I've tried but failed with that

@tomasciccola tomasciccola self-assigned this Aug 2, 2023
@tomasciccola tomasciccola changed the title Convertion functions for authStore types Conversion functions for authStore types Aug 2, 2023
@tomasciccola tomasciccola linked an issue Aug 2, 2023 that may be closed by this pull request
3 tasks
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I think the enums with _ are fine.

I think the JSONSchema for Role should include UNRECOGNIZED as an enum option, rather than trying to use a default, and the client should handle that accordingly. This is for future compatibility so that a new action does not break existing clients.

Needs changes from master merged in, for the const: schemaName change.

@tomasciccola
Copy link
Contributor Author

Looks good to me, I think the enums with _ are fine.

I think the JSONSchema for Role should include UNRECOGNIZED as an enum option, rather than trying to use a default, and the client should handle that accordingly. This is for future compatibility so that a new action does not break existing clients.

Needs changes from master merged in, for the const: schemaName change.

Ok, I see that, for example, the Field schema doesn't have an UNRECOGNIZED option on the type field (only text|number|selectOne|selectMultiple) in what sense that case is different? mostly because we're certain we are not going to add additional options in the future??

@tomasciccola tomasciccola requested a review from gmaclennan August 2, 2023 18:40
@gmaclennan
Copy link
Member

Ok, I see that, for example, the Field schema doesn't have an UNRECOGNIZED option on the type field (only text|number|selectOne|selectMultiple) in what sense that case is different? mostly because we're certain we are not going to add additional options in the future??

Ah good point. I initially had initially thought that a text field is a good fallback for anything, but because the actual values stored in the tags could be different, I think it's better for the client to decide how to deal with this, by which I mean that UNRECOGNIZED is a good option here - if the client sees that, it can look at any existing value and choose the best way to display it.

It's a different case with the appearance option, where multiline is a good fallback for any future options.

Base automatically changed from feat/add-configstore-fields to master August 3, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add encode/decode conversion functions for auth core types
2 participants